Add service interface to control hardware lifecycle #559
Conversation
destogl
force-pushed
the
services-for-hardware-lifecycle
branch
from
bbf2f64
to
a14d222
Compare
destogl
force-pushed
the
services-for-hardware-lifecycle
branch
from
7a22f68
to
58d5cf8
Compare
There was a problem hiding this comment.
Too much.
Too many lines of code.
Too many things in one PR: new services and overhaul of resource manager to support special movement interfaces.
Let's work out how to split this PR, I'd really hate to see this work wasted but I am not feeling good about this amount of changes in a single PR.
- You have a huge surface for conflicts
- It takes a long time to review the code
- Had to take breaks during review to avoid brain meltdown
Which does not leave me with a good feeling regarding the quality of my review.
Here's a list of PRs I propose:
- controller_manager_msgs, add new service interfaces with an empty implementation and no tests
- API changes for init/configure/hardware component info
- misc changes that shouldn't affect things, aka extending the implementations of
- test_actuator
- test_generic_system
- test_system
- functional tests and hardware lifecycle management implementation + docs
- "movement interface"-related changes, once we figure out what we actually envision there
|
|
||
| std::vector<std::string> autoconfigure_components = {""}; | ||
| get_parameter("autoconfigure_components", autoconfigure_components); | ||
| rclcpp_lifecycle::State inactive_state( |
There was a problem hiding this comment.
| rclcpp_lifecycle::State inactive_state( | |
| const rclcpp_lifecycle::State inactive_state( |
why do we even have to write constants like this? there should be some global consts available like rclcpp_lifecycle::State::Inactive
There was a problem hiding this comment.
Because the upstream library does not defines standard states fully, at least not with the ID or names. We could create defines for this or should we directly try to add this into rclcpp?
There was a problem hiding this comment.
Also it is not possible to set this const because of the underlying logic where no copy of state is done but in place changes are added if ID is not provided... Will comment more in corresponding PR.
| {{false, false, false, false}, {false, false, false, false, false, false, false}}, // system | ||
| })); | ||
|
|
||
| // auto test_controller = std::make_shared<test_controller::TestController>(); |
There was a problem hiding this comment.
please either fix or remove this 100 lines of commented code
There was a problem hiding this comment.
We are missing here test where controllers are used. This was there as a reminder. Now I have this comment which should stay unresolved until we fix this or open an issue.
| EXPECT_NO_THROW(rm.claim_state_interface("external_joint/external_state_interface")); | ||
| EXPECT_NO_THROW(rm.claim_command_interface("external_joint/external_command_interface")); | ||
| // TODO(destogl): repair this test! | ||
| // It fails because interfaces are not in available lists |
There was a problem hiding this comment.
so... do we repair or remove it? follow-up issue? fix now?
hardware_interface/test/fake_components/test_generic_system.cpp
Outdated
Show resolved
Hide resolved
| #define COMPONENT_NAME_COMPARE_INV \ | ||
| [&](const auto & component) { return component.get_name() == component_name; } | ||
|
|
||
| #define MOVEMENT_INTERFACES_COMPARE \ |
There was a problem hiding this comment.
why a define on these and not a simple using or just assigning the lambda toa const var?
There was a problem hiding this comment.
This is because the lambda is outside of class and I would like to have access to the current context "[&]". Will check if there is something better to do here. Although I don't think so.
| // check if interface is non-movement interface | ||
| if ( | ||
| std::find_if( | ||
| MOVEMENT_INTERFACES.begin(), MOVEMENT_INTERFACES.end(), MOVEMENT_INTERFACES_COMPARE) == |
There was a problem hiding this comment.
I'm really not happy about us creating/maintaining a standard set of movement interfaces that is handled specially.
The default should always be movement interfaces. How hardware components handle/allow for non-movement interfaces should be designed carefully and not in this PR. I'm sorry but I think the best option at this point is to remove everything related to movement interfaces from this PR and we should discuss the design for this instead. This is way too much code IMO to properly assess and maintain every behaviour of this without a design.
|
|
||
| if (result) | ||
| { | ||
| // TODO(destogl): change this - deimport all things if there is there are interfaces there |
There was a problem hiding this comment.
Sorry to be blunt but no. I'm not going to add more comments to this file.
At this point this file has way too many TODOs, we should not leave holes and doubts and todos in any feature implementation. All in all, there should be less code in a PR.
…'stop' -> 'deactivate'
Add HardwareComponentInfo structure in resource manager. Use constants for HW parameters in tests of resource_manager. Add list hardware components in CM to get details about them and check their status. Use clear name for 'guard' and move release cmd itfs for better readability. RM: Add lock for accesing maps with stored interfaces. Separate hardware components-related services after controllers-related services. Add service for activate/deactive hardware components. Add activation and deactivation through ResourceStorage. This helps to manage available command interfaces.
…only 'set_hardware_component_state' service.
destogl
force-pushed
the
services-for-hardware-lifecycle
branch
from
1e897a4
to
6c11ff8
Compare
This PR adds lifecycle management of hardware through services of CM and additional logic to RM to support lifecycle.
@bmagyar: should we delete individual services like "configure_hardware" and leave only. "set_hardware_component_state"?NOTE: This PR is replaced with multiple follow-ups, but it should stay open until everything is merged because it provides a fully-functional version.